Skip to content

side_diff: fix the column layout overflow and --expand-tabs - #270

Open
sami-daniel wants to merge 8 commits into
uutils:mainfrom
sami-daniel:fix/264-side-diff-layout-overflow
Open

side_diff: fix the column layout overflow and --expand-tabs#270
sami-daniel wants to merge 8 commits into
uutils:mainfrom
sami-daniel:fix/264-side-diff-layout-overflow

Conversation

@sami-daniel

Copy link
Copy Markdown
Contributor

Fixes #264.

Side-by-side sized its columns with signed arithmetic on the --tabsize value, which is parsed with no upper bound. A large tab size overflowed, and what happened next depended on the build: with -C overflow-checks=on the process aborted, and in release the value wrapped and the columns came out with bogus widths.

$ printf 'a\n' > f1; printf 'b\n' > f2
$ diff -y --tabsize=9223372036854775805 f1 f2
thread 'main' panicked at src/side_diff.rs:85:24:
attempt to add with overflow

A tab size past the signed range never aborted. It cast to a negative number and sized the columns as if the tab size were small, so the output looked ordinary and was wrong. The calculation is now unsigned throughout and holds for every input on its own, instead of relying on validation upstream.

--expand-tabs was accepted but not taken into account when sizing the columns. Column two has to start on a tab stop only so that tabs in the right column keep their position relative to the stops, and once tabs are expanded there are none left to keep aligned.

This changes the output of -y -t. At the default width the separator moves from column 62 to 64.

That wider half line exposed a third overflow, introduced and fixed in the same commit. A large tab size used to leave the half line empty, so nothing was ever drawn; with -t the line now fits, and the sums that walk to the next tab stop overflowed. They measure the step against the room that is left instead of adding absolute columns.

The side-by-side layout tests need parameterized cases so that each
width and tab size shows up as its own named test instead of a loop
that reports a single failure with no indication of which input broke.
`Config::new` sized the side-by-side columns with signed arithmetic on
the `--tabsize` value, which `params.rs` accepts with no upper bound.
`tab_size + GUTTER_WIDTH_MIN` overflowed for a large one: with
`-C overflow-checks=on` that aborted, and in release it wrapped and drew
bogus widths. A value above `isize::MAX` did not even abort, it cast to a
negative number and silently laid the columns out as if the tab size were
small. Closes uutils#264.

The calculation moves into `Config::layout`, in `usize` throughout, and
holds for every input on its own rather than relying on validation
upstream:

- An early return for `tab_size > full_width`. This is exact, not a
  clamp: the offset is a multiple of `tab_size`, so it is either zero or
  already past the right edge, and neither leaves room for a second
  column. It is also what keeps the sum below in range.
- `saturating_add` for the gutter sum, which covers the one case the
  strict `>` lets through, `full_width == tab_size == usize::MAX`.
- `saturating_sub` for the two bounds that could go negative.
  `full_width = 5, tab_size = 8` really does produce an offset past the
  right edge.
- `usize::midpoint` for the balance point, which cannot overflow.
- `saturating_sub` on `separator_pos`, which underflowed for a zero
  width. Unreachable from the CLI, reachable through the library.

A tab stop every zero columns has no meaning and every reader of
`tab_size` divides by it, so `Config::new` now normalizes it to one. The
layout alone was not enough: fixing it only moved the division by zero
from `layout` to `format_tabs_and_spaces`.

The new `mod layout` covers the boundaries, including the two that pin
the design down: `full_width = 10, tab_size = 8` still leaves two
columns, so no looser guard is correct, and `tab_size == full_width`
yields a non-zero offset with an empty half line.
`Config::new` took `expanded` but ignored it when sizing the columns. The
manual's "Preserving Tab Stop Alignment" section explains why it matters:
column two has to start on a tab stop only so that tabs in the right
column keep their position relative to the stops. With `--expand-tabs`
there are no tabs left in the output, so there is no grid to preserve and
every column is a stop.

Compared against the output of `diff` from GNU diffutils 3.10. The
separator column now matches it for `-y -t` at widths 40, 80 and 130, and
for `-y -t --tabsize=4 --width=100`: columns 19, 39, 64 and 49. At the
default width it sat on column 62 before, two columns off.

The tab size still decides how far an expanded tab reaches, so it stays in
the field and only `layout` sees the 1. Feeding the 1 into the field would
shrink every expanded tab to a single space.

The widened half line makes a crash reachable. A tab size near
`usize::MAX` used to yield an empty half line, so `process_half_line`
returned before drawing anything; with `-t` the line now fits and three
expressions of the form
`current_width + tab_size - (current_width % tab_size)` overflowed.
`format_tabs_and_spaces` and the tab arm now measure the step to the next
stop against the room that is left instead of summing absolute columns,
which the surrounding `current_width <= max_width` already bounds.

`test_full_width_40_tab_8` used `expanded = true` and expected the widths
computed while ignoring it. The separator lands on column 19 either way,
so only the half width and the offset change.

The new tests were checked by mutation. Dropping the ternary breaks
`expanded_tabs_widen_the_half_line`,
`expanded_tabs_lay_out_as_a_stop_on_every_column` and
`test_full_width_40_tab_8`; applying it to the field instead breaks
`expanded_tabs_keep_the_real_tab_size_for_rendering` and
`expanded_tabs_reach_the_next_real_tab_stop`.
`width` and `tabsize` were commented out of `fuzz_side`, along with a
`width == 0 || tabsize == 0` early return that was commented out too,
because the column arithmetic could not take arbitrary values. It can
now, so both are fed to the target and neither guard is needed.

They are `u16` rather than `usize`: the layout handles the whole range,
but a width near `usize::MAX` asks the renderer for petabytes of
padding, which would only produce timeouts.
@sami-daniel sami-daniel changed the title side_diff: fix the column layout overflow side_diff: fix the column layout overflow and --expand-tabs Aug 13, 2026
The assertions state the widths already, and the surrounding test name
carries the tab size.
A carriage return pads all the way to column two, the only caller that
reaches the far end of the line. That walk used to add absolute columns
and overflowed with `--width` and `--tabsize` at the top of the range,
which aborts under `-C overflow-checks=on`.

Both values sit at the maximum on purpose: a smaller tab size walks to
that end one stop at a time, and a smaller width never reaches the sum
that overflowed. Expansion stays off, since it pads with spaces one
column at a time and would not finish at this width.
@sami-daniel
sami-daniel marked this pull request as ready for review August 13, 2026 03:43
@github-actions

Copy link
Copy Markdown

GNU diffutils testsuite comparison:

Test results comparison:
  Current:   TOTAL: 33 / PASSED: 0 / FAILED: 33 / SKIPPED: 0
  Reference: TOTAL: 33 / PASSED: 8 / FAILED: 21 / SKIPPED: 4

Changes from main branch:
  TOTAL: +0
  PASSED: -8
  FAILED: +12

New test failures (12):
  - basic
  - bignum
  - brief-vs-stat-zero-kernel-lies
  - bug-64316
  - cmp
  - diff3
  - help-version
  - large-subopt
  - strcoll-0-names
  - strip-trailing-cr
  - timezone
  - y2038-vs-32bit

A tab size wider than the line collapses the layout to a single column
and draws nothing, so the whole `usize` range costs no more than a small
one. Capping it at `u16` left out the values above `isize::MAX`, which
are the ones the old signed arithmetic turned negative.

The width stays a `u16`, since the padding it asks for is written one
column at a time and the cost scales with it.
Superscript digits do not survive every terminal or editor the file gets
read in.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

diff: -y --tabsize=<huge> overflows the column arithmetic (overflow-checks only)

1 participant